-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: replacing all generic errors with FuelError
#2549
base: master
Are you sure you want to change the base?
chore: replacing all generic errors with FuelError
#2549
Conversation
@msensys does this pick up all occurences? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msensys does this pick up all occurences?
It believe not - hence the "Relates to"?
Sorry @msensys was just trying to validate why the validate changeset CI stage hadn't run so converted to draft |
Maybe not. I will send some commits to pick up all occurrences. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work!
To fully wrap this up, can you please go to the tests that are expecting these errors and use the expectToThrowFuelError
utility?
For example, for this error, there exists this test. You'd change the test to look like this:
it('throws when setting configurable but predicate has none', async () => {
await expectToThrowFuelError(
() =>
new Predicate({
bytecode: predicateBytesTrue,
abi: predicateAbiTrue,
provider: wallet.provider,
inputData: ['NADA'],
configurableConstants: {
constant: 'NADA',
},
}),
new FuelError(
ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
'Predicate has no configurable constants to be set'
)
);
});
You can do a repo-wide search for the expected error message on each error you changed and there should be a corresponding test which you'd change as shown above. If a test doesn't exist for the error, you're welcome to add it.
Hey @nedsalk! The way that I found to resolve the problem of the test was this: it('throws when setting configurable but predicate has none', async () => {
await expectToThrowFuelError(
() =>
new Predicate({
bytecode: predicateBytesTrue,
abi: predicateAbiTrue,
provider: wallet.provider,
inputData: ['NADA'],
configurableConstants: {
constant: 'NADA',
},
}),
{
code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
message: expect.stringContaining('Predicate has no configurable constants to be set'),
}
);
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msensys It looks like this PR will actually close this, correct?
If so, please change the description to Close #NNN
instead of Relates to #NNN
.
Also, if you add a comment on #2001, we can assign it to you.
@arboleya, can I consider it as I associated this as |
@msensys Oh, I assumed it would handle all generic errors, not only a subset. Isn't that the case? If not, it won't be enough to close #2001. |
I'll cover them all, but I want to know about the name to proceed :D |
@msensys What do you mean by "name"? |
Change the label of commit. Btw, some tests use |
{ | ||
code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS, | ||
message: expect.stringContaining('Predicate has no configurable constants to be set'), | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msensys the expectToThrowFuelError
utility internally checks for message
(and any other property) equality. You shouldn't be passing this as its second argument:
{
code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
message: expect.stringContaining('Predicate has no configurable constants to be set'),
}
but rather this:
new FuelError(
ErrorCode.INVALID_CONFIGURABLE_CONSTANTS,
'Predicate has no configurable constants to be set'
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know why, but the test isn't passing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nedsalk look at here:
- [FuelError: Predicate has no configurable constants to be set]
+ Object {
+ "VERSIONS": Object {
+ "FORC": "0.60.0",
+ "FUELS": "0.90.0",
+ "FUEL_CORE": "0.30.0",
+ },
+ "code": "invalid-configurable-constants",
+ "metadata": Object {},
+ "name": "FuelError",
+ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any failing tests; CI is green for all workflows.
But as @nedsalk said, you should not assert the error with an Object:
fuels-ts/packages/fuel-gauge/src/predicate/predicate-configurables.test.ts
Lines 211 to 214 in 06aadf6
{ | |
code: ErrorCode.INVALID_CONFIGURABLE_CONSTANTS, | |
message: expect.stringContaining('Predicate has no configurable constants to be set'), | |
} |
Instead, you should use FuelError
, as you did on the other occurrences:
fuels-ts/packages/fuel-gauge/src/predicate/predicate-configurables.test.ts
Lines 230 to 233 in 06aadf6
new FuelError( | |
ErrorCode.INVALID_CONFIGURABLE_CONSTANTS, | |
`Error setting configurable constants: No configurable constant named 'NOPE' found in the Predicate.` | |
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I committed the change you mentioned. Take a look at CI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@msensys the test is failing because of this line catching and rethrowing the error with an updated message. To fix the failing test, change the expected message in your test to Error setting configurable constants: Predicate has no configurable constants to be set.
; you were missing the Error setting configurable constants:
part. We should maybe rethink this try/catch here, but it's out of the scope of this issue. Just changing the expected message is enough.
FuelError
Co-authored-by: Anderson Arboleya <anderson@arboleya.me>
@msensys I see that this PR still needs some work as there are quite a few places we still |
Note that we should use the However, to apply this, you'll need to wait until we merge this:
|
Also, |
@mvares did you need any assistance with this one? |
Some |
Error
toFuelError
#2001